🦋 Changeset detected

Latest commit: e44bb99

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

The only reason why the tests are failing seems unrelated to my code:

We usually use examples for beginners who want to start from somewhere. Having a custom renderer is not a beginner example, so I don't think we should have it. Instead, we should have tests that cover the new changes.

@ematipico Right, I was foreseeing this argument somehow :)) I'll take the example code and put it in a new repo under my Github account. People who plan to implement their own custom renderers would be able to find our conversation, and start from there.

I'll also add a test to verify my changes, but due to the nature of the issue, I need to temporarily hook the console.warn function in order to prove that the log is not written anymore.

If that can help, in this commit I removed a patch to intercept console logs. Maybe you can reuse it in some way

Moved the example code for a custom renderer / custom frontend framework integration over here: https://github.com/kyr0/astro-custom-renderer

CodSpeed Performance Report

Merging #12461 will degrade performances by 35.31%

_{Comparing kyr0:fix/custom-renderer-warning (e44bb99) with main (97f413f)}

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 3 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

To be honest: I don't believe so. Removing a console.warn certainly wouldn't drop the performance by 11%; especially not if the codepath affected by the report hasn't be changed at all.

It seems like CodSpeed has a higher variance than promised?

Moved the example code for a custom renderer / custom frontend framework integration over here: kyr0/astro-custom-renderer

If you plan to advertise your custom renderer example, I would suggest to change the entry points to use a valid extension. Currently they don't have one, which is invalid in Node.js. Every import must have a file extension. In fact, even our container API suggests the use of the extension

Moved the example code for a custom renderer / custom frontend framework integration over here: kyr0/astro-custom-renderer

If you plan to advertise your custom renderer example, I would suggest to change the entry points to use a valid extension. Currently they don't have one, which is invalid in Node.js. Every import must have a file extension. In fact, even our container API suggests the use of the extension

The reason why it's actually not that technically "invalid" in this specific case, and worked (even if it looks a bit invalid, I agree!) was that this project uses TypeScript path aliases, as defined in the tsconfig.json file (link).

At transpile/bundle ("compile") time, during development, Vite uses esbuild to transpile TypeScript files and resolve the aliases. For production builds, rollup handles path resolution and optimizations as part of the bundling step, ensuring entry points are correctly adjusted or the code is inlined. Those steps are invisible to the developer (the generated code only exists on-the-fly) when the custom renderer is part of an Astro projects codebase.

Because the code generation is on-the-fly in this case, the change you requested, is not as easy as simply adding a .js, because the .js files for these entrypoints don't exist ahead of time. They are not resolvable at the time of the integration execution. Only the .ts files do exists because there is no bundle step in between. When using path aliases, Vite (as described above) can resolve them and handles transpilation and referencing well - also on-the-fly. Now, in the end, the generated code is valid, when Node.js is executing it. I agree that this is a bit weird for a repo that's meant to be an example.

Changing this (not relying on path aliases) requires a major rework of the example, now with a monorepo structure where the renderer would get its own package, exports, dist build output, and the actual astro example could depend on it. Because Vite would be able to load the built dist files of the module, loading the entrypoints with a .js extension added would work.

It makes sense to implement this structural change, also, because most developers who'd plan to implement their custom renderers or framework integrations wouldn't do it inside of an example Astro project repository. I just thought the example would look way easier without a more complicated monorepo setup. But I agree that in this case, correctness is much more important.

edit:

Refactoring done: https://github.com/kyr0/astro-custom-renderer

Using .js, no path aliases anymore: https://github.com/kyr0/astro-custom-renderer/blob/main/packages/custom-renderer/src/index.ts#L5

@ematipico @bluwy Do you think this is ready to get merged? Is there anything I can do to help further? Should I remove the test as suggested?

@bluwy I removed the guarding test. Just the CodSpeed..